-
Notifications
You must be signed in to change notification settings - Fork 107
Add skeleton for the get_block_hash syscall. #568
Add skeleton for the get_block_hash syscall. #568
Conversation
9f442ae
to
c04af81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 6 files at r1.
Reviewable status: 1 of 6 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware and @Yoni-Starkware)
crates/blockifier/feature_contracts/cairo1/test_contract.cairo
line 48 at r1 (raw file):
#[external] fn test_get_block_hash(block_number: felt252) -> felt252 {
Why is there a difference from here? (felt252
vs. u64
)
Code quote:
block_number: felt252
crates/blockifier/src/execution/syscalls/hint_processor.rs
line 317 at r1 (raw file):
// TODO(Arni, 6/6/2023): Implement this logic. pub fn get_block_hash(&mut self, _block_number: StarkFelt) -> SyscallResult<StarkFelt> { Err(SyscallExecutionError::Unimplemented)
Why is it needed? Why can't you implement the logic directly in the file mod.rs
? Do you need the syscall_handler
for the implementation (why is it a method)?
Code quote:
// TODO(Arni, 6/6/2023): Implement this logic.
pub fn get_block_hash(&mut self, _block_number: StarkFelt) -> SyscallResult<StarkFelt> {
Err(SyscallExecutionError::Unimplemented)
crates/blockifier/src/execution/syscalls/hint_processor.rs
line 317 at r1 (raw file):
// TODO(Arni, 6/6/2023): Implement this logic. pub fn get_block_hash(&mut self, _block_number: StarkFelt) -> SyscallResult<StarkFelt> { Err(SyscallExecutionError::Unimplemented)
Why is it needed? Why can't you return some value?
Code quote:
Err(SyscallExecutionError::Unimplemented)
crates/blockifier/src/execution/syscalls/syscalls_test.rs
line 117 at r1 (raw file):
}; // TODO(spapini): Fix the "UNEXPECTED ERROR".
Why? WDYM?
Code quote:
// TODO(spapini): Fix the "UNEXPECTED ERROR".
crates/blockifier/src/fee/os_resources.rs
line 52 at r1 (raw file):
"n_memory_holes": 0, "n_steps": 0 },
Out of curiosity - where do these numbers come from?
Code quote:
"GetBlockHash": {
"builtin_instance_counter": {},
"n_memory_holes": 0,
"n_steps": 0
},
c04af81
to
8cc92e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 7 files reviewed, 8 unresolved discussions (waiting on @ArniStarkware and @Yoni-Starkware)
crates/blockifier/feature_contracts/cairo1/test_contract.cairo
line 46 at r2 (raw file):
starknet::syscalls::emit_event_syscall(keys.span(), data.span()).unwrap_syscall(); }
Compile the contract? :)
crates/blockifier/src/execution/syscalls/mod.rs
line 309 at r2 (raw file):
let contract_address = ContractAddress::try_from(StarkFelt::from(constants::BLOCK_NUMBER_TO_BLOCK_HASH_ADDRESS))?; let block_hash = syscall_handler.state.get_storage_at(contract_address, key)?;
See get_contract_storage_at()
. You need to update the accessed_keys
and read_values
values.
Code quote:
let block_hash = syscall_handler.state.get_storage_at(contract_address, key)?;
crates/blockifier/src/execution/syscalls/syscalls_test.rs
line 117 at r2 (raw file):
}; let _call_info = entry_point_call.execute_directly(&mut state).unwrap();
I don't know what should be the value. I think you will need to declare and deploy the contract at BLOCK_NUMBER_TO_BLOCK_HASH_ADDRESS
(and initialize the key 1800
).
If you don't want to do this in this PR, please add a TODO.
Suggestion:
assert_eq!(entry_point_call.execute_directly(&mut state).unwrap(). execution, CallExecution::from_retdata(retdata![stark_felt!(value)]))
Previously, noaov1 (Noa Oved) wrote…
Done. Removed. |
8cc92e0
to
3524621
Compare
3524621
to
2fbc5d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 9 files reviewed, 8 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware)
crates/blockifier/feature_contracts/cairo1/test_contract.cairo
line 48 at r1 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why is there a difference from here? (
felt252
vs.u64
)
Done. TY for sending an example.
crates/blockifier/feature_contracts/cairo1/test_contract.cairo
line 46 at r2 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Compile the contract? :)
Done. Maybe we will want to compile again.
crates/blockifier/src/execution/syscalls/hint_processor.rs
line 317 at r1 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why is it needed? Why can't you implement the logic directly in the file
mod.rs
? Do you need thesyscall_handler
for the implementation (why is it a method)?
Done. Removed.
crates/blockifier/src/execution/syscalls/hint_processor.rs
line 317 at r1 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why is it needed? Why can't you return some value?
Done. removed.
crates/blockifier/src/execution/syscalls/syscalls_test.rs
line 117 at r2 (raw file):
Previously, noaov1 (Noa Oved) wrote…
I don't know what should be the value. I think you will need to declare and deploy the contract at
BLOCK_NUMBER_TO_BLOCK_HASH_ADDRESS
(and initialize the key1800
).
If you don't want to do this in this PR, please add a TODO.
Done.
crates/blockifier/src/fee/os_resources.rs
line 52 at r1 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Out of curiosity - where do these numbers come from?
Still needs to be done.
Previously, noaov1 (Noa Oved) wrote…
Do you mean I need to initialize the mapping? I agree - in another PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r4, all commit messages.
Reviewable status: 2 of 9 files reviewed, 9 unresolved discussions (waiting on @ArniStarkware and @noaov1)
crates/blockifier/src/abi/constants.rs
line 40 at r4 (raw file):
// This contract stores the block number -> block hash mapping. pub const BLOCK_HASH_CONTRACT_ADDRESS: u64 = 0x1;
Suggestion:
pub const BLOCK_HASH_CONTRACT_ADDRESS: u64 = 1;
crates/blockifier/src/execution/syscalls/mod.rs
line 309 at r2 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
Do you mean I need to initialize the mapping? I agree - in another PR.
Actually no, it's different.
Previously, Yoni-Starkware (Yoni) wrote…
We do not need to update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 9 files reviewed, 9 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware)
crates/blockifier/src/abi/constants.rs
line 40 at r4 (raw file):
// This contract stores the block number -> block hash mapping. pub const BLOCK_HASH_CONTRACT_ADDRESS: u64 = 0x1;
Done.
7fd6850
to
a30aa9e
Compare
2fbc5d8
to
7b0dacd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 9 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware and @Yoni-Starkware)
crates/blockifier/src/abi/constants.rs
line 40 at r4 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
Done.
Or maybe pub const BLOCK_HASH_CONTRACT_ADDRESS: &str = "0x1";
crates/blockifier/src/execution/syscalls/mod.rs
line 309 at r2 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
We do not need to update
accessed_keys
andread_values
. This is a read from one contract to "another contract" - which is not actually a contract, but a ledger for this particular history.
You are right, we don't need to update the those mappings.
crates/blockifier/src/execution/syscalls/mod.rs
line 322 at r5 (raw file):
request: GetBlockHashRequest, _vm: &mut VirtualMachine, syscall_handler: &mut SyscallHintProcessor<'_>,
I wonder why it wasn't indicated by the compiler
Suggestion:
pub fn get_block_hash(
request: GetBlockHashRequest,
_vm: &mut VirtualMachine,
syscall_handler: &mut SyscallHintProcessor<'_>,
_remaining_gas: &mut Felt252,
crates/blockifier/src/execution/syscalls/syscalls_test.rs
line 117 at r2 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
Done.
I wonder what will enforce that the BLOCK_HASH_CONTRACT
is deployed and that the queried block number was initialized. Do we want that?
7b0dacd
to
733efc1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 9 files reviewed, 5 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware)
crates/blockifier/src/execution/syscalls/mod.rs
line 309 at r2 (raw file):
Previously, noaov1 (Noa Oved) wrote…
You are right, we don't need to update the those mappings.
Done.
crates/blockifier/src/execution/syscalls/mod.rs
line 322 at r5 (raw file):
Previously, noaov1 (Noa Oved) wrote…
I wonder why it wasn't indicated by the compiler
It was. You are right. Done.
Suggestion: // Raises an error if latest_block_number - block_number < 10. |
733efc1
to
8019cc6
Compare
a30aa9e
to
fdf091f
Compare
Previously, noaov1 (Noa Oved) wrote…
Regarding the "Is the queried block was initialized" question: This is somewhat addressed in the TODO in |
fdf091f
to
dc17266
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 9 files reviewed, 6 unresolved discussions (waiting on @ArniStarkware, @dafna, and @Yoni-Starkware)
crates/blockifier/src/execution/syscalls/mod.rs
line 290 at r7 (raw file):
#[derive(Debug, Eq, PartialEq)] // TODO(Arni, 8/6/2023): Consider replacing with block_number: u64 or with BlockNumber.
Suggestion:
// TODO(Arni, 8/6/2023): Consider replacing with `BlockNumber`.
crates/blockifier/src/execution/syscalls/mod.rs
line 304 at r7 (raw file):
#[derive(Debug, Eq, PartialEq)] pub struct GetBlockHashResponse { pub block_hash: StarkFelt,
Suggestion:
BlockHash
crates/blockifier/src/execution/syscalls/mod.rs
line 318 at r7 (raw file):
// Returns the expected block hash if 10 <= latest_block_hash - block_number < 1024. // Returns unexpected value, otherwise (Most likly - returns an uninitialized value = 0). // TODO(Arni, 11/6/2023): Implement according to the documentation above.
What is the latest_block_number
? The last one that was created?
Suggestion:
// Returns the block hash of a given block_number.
// Returns the expected block hash if the given block was created at most 1024 blocks before the latest block and at least 10 blocks before.
// Otherwise, returns a default value or an error respectively.
// TODO(Arni, 11/6/2023): Implement according to the documentation above.
crates/blockifier/src/execution/syscalls/syscalls_test.rs
line 117 at r2 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
Regarding the "Is the queried block was initialized" question: This is somewhat addressed in the TODO in
crates/blockifier/src/execution/syscalls/mod.rs
.
Regarding the enforcement thatBLOCK_HASH_CONTRACT
is deployed, this is @dafna Matsry's domain.
I don't follow :) Will you check whether the returned value is different from 0?
crates/blockifier/src/fee/os_resources.rs
line 52 at r1 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
Still needs to be done.
Add a TODO?
8019cc6
to
7905ba6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 5 files at r8, all commit messages.
Reviewable status: 3 of 9 files reviewed, 8 unresolved discussions (waiting on @ArniStarkware, @dafna, and @noaov1)
crates/blockifier/src/abi/constants.rs
line 64 at r8 (raw file):
// This contract stores the block number -> block hash mapping. pub const BLOCK_HASH_CONTRACT_ADDRESS: u64 = 1;
u64 -> Address type or whatever it is called here
Code quote:
const BLOCK_HASH_CONTRACT_ADDRESS: u64 = 1;
crates/blockifier/src/execution/deprecated_syscalls/mod.rs
line 75 at r8 (raw file):
b"Deploy" => Ok(Self::Deploy), b"EmitEvent" => Ok(Self::EmitEvent), b"GetBlockHash" => Ok(Self::GetBlockHash),
Why? it's the deprecated handler
Code quote:
b"GetBlockHash" => Ok(Self::GetBlockHash),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 9 files reviewed, 8 unresolved discussions (waiting on @dafna, @giladchase, @noaov1, and @Yoni-Starkware)
crates/blockifier/src/abi/constants.rs
line 64 at r8 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
u64 -> Address type or whatever it is called here
You wish that the type of the constant will be: ContractAddress
.
Rust complaints:
error[E0015]: cannot call non-const fn `<starknet_api::hash::StarkFelt as std::convert::From<u64>>::from` in constants
--> crates/blockifier/src/execution/syscalls/mod.rs:30:43
|
30 | pub const BLOCK_HASH_ADDRESS: StarkFelt = StarkFelt::from(1u64);
| ^^^^^^^^^^^^^^^^^^^^^
|
= note: calls in constants are limited to constant functions, tuple structs and tuple variants
Spoke with @giladchase. Solving this may be overkill. here is some code, allowing for similar behavior using lazy
.
This change requires adding crates. Will try to do that if I have time.
crates/blockifier/src/execution/deprecated_syscalls/mod.rs
line 75 at r8 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Why? it's the deprecated handler
IIUC, this is a unified list for both deprecated and not deprecated syscalls. Some sort of Hack. @noaov1 - assured me this is the correct thing to do.
crates/blockifier/src/execution/syscalls/mod.rs
line 290 at r7 (raw file):
#[derive(Debug, Eq, PartialEq)] // TODO(Arni, 8/6/2023): Consider replacing with block_number: u64 or with BlockNumber.
Done.
crates/blockifier/src/execution/syscalls/mod.rs
line 304 at r7 (raw file):
#[derive(Debug, Eq, PartialEq)] pub struct GetBlockHashResponse { pub block_hash: StarkFelt,
Done.
crates/blockifier/src/execution/syscalls/mod.rs
line 318 at r7 (raw file):
Previously, noaov1 (Noa Oved) wrote…
What is the
latest_block_number
? The last one that was created?
Done.
crates/blockifier/src/execution/syscalls/syscalls_test.rs
line 117 at r2 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Please check a non-trivial value instead.
Testing a non-trivial case.
Done.
crates/blockifier/src/fee/os_resources.rs
line 52 at r1 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
To start with, copy the values of
storage_read
, which is quite similar, and add a TODO.
Done.
2704651
to
dc17266
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 5 files at r8, all commit messages.
Reviewable status: 4 of 9 files reviewed, 7 unresolved discussions (waiting on @dafna and @noaov1)
crates/blockifier/src/abi/constants.rs
line 64 at r8 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
You wish that the type of the constant will be:
ContractAddress
.Rust complaints:
error[E0015]: cannot call non-const fn `<starknet_api::hash::StarkFelt as std::convert::From<u64>>::from` in constants --> crates/blockifier/src/execution/syscalls/mod.rs:30:43 | 30 | pub const BLOCK_HASH_ADDRESS: StarkFelt = StarkFelt::from(1u64); | ^^^^^^^^^^^^^^^^^^^^^ | = note: calls in constants are limited to constant functions, tuple structs and tuple variants
Spoke with @giladchase. Solving this may be overkill. here is some code, allowing for similar behavior using
lazy
.This change requires adding crates. Will try to do that if I have time.
No need, I see that they use u64 also for selectors, so let's go with their convention (u64 is fine)
dc17266
to
2704651
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 9 files reviewed, 8 unresolved discussions (waiting on @ArniStarkware, @dafna, and @Yoni-Starkware)
crates/blockifier/src/execution/syscalls/mod.rs
line 333 at r9 (raw file):
syscall_handler.state.get_storage_at(block_hash_contract_address, key)?, ), })
Suggestion:
let block_hash = BlockHash(syscall_handler.state.get_storage_at(block_hash_contract_address, key)?)
Ok(GetBlockHashResponse {
block_hash
})
crates/blockifier/src/execution/syscalls/syscalls_test.rs
line 133 at r9 (raw file):
// Initialize block number -> block hash entry. let block_number = stark_felt!(1800_u64); let block_hash = stark_felt!(0x42_u64);
Suggestion:
"0x42"
crates/blockifier/src/execution/syscalls/syscalls_test.rs
line 147 at r9 (raw file):
}; // Run entry point and test result.
I think you can remove it.
Code quote:
// Run entry point and test result.
crates/blockifier/src/execution/syscalls/syscalls_test.rs
line 151 at r9 (raw file):
entry_point_call.execute_directly(&mut state).unwrap().execution, CallExecution { gas_consumed: stark_felt!(0x3d86_u128),
Can you please convert it to an integer? As in the rest of the tests (for consistency)
Code quote:
0x3d86_u128
fd76200
to
b0ed9f9
Compare
b0ed9f9
to
51f42e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 23 files reviewed, 8 unresolved discussions (waiting on @dafna, @noaov1, and @Yoni-Starkware)
crates/blockifier/src/execution/syscalls/mod.rs
line 333 at r9 (raw file):
syscall_handler.state.get_storage_at(block_hash_contract_address, key)?, ), })
Done.
crates/blockifier/src/execution/syscalls/syscalls_test.rs
line 133 at r9 (raw file):
// Initialize block number -> block hash entry. let block_number = stark_felt!(1800_u64); let block_hash = stark_felt!(0x42_u64);
Done.
crates/blockifier/src/execution/syscalls/syscalls_test.rs
line 147 at r9 (raw file):
Previously, noaov1 (Noa Oved) wrote…
I think you can remove it.
Done.
crates/blockifier/src/execution/syscalls/syscalls_test.rs
line 151 at r9 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Can you please convert it to an integer? As in the rest of the tests (for consistency)
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 12 files at r13, all commit messages.
Reviewable status: 4 of 23 files reviewed, 9 unresolved discussions (waiting on @ArniStarkware, @dafna, and @noaov1)
crates/blockifier/src/execution/syscalls/mod.rs
line 294 at r14 (raw file):
// TODO(Arni, 8/6/2023): Consider replacing `BlockNumber`. pub struct GetBlockHashRequest { pub block_number: StarkFelt,
Why not use u64? Like in Cairo 1.
Code quote:
// TODO(Arni, 8/6/2023): Consider replacing `BlockNumber`.
pub struct GetBlockHashRequest {
pub block_number: StarkFelt,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 23 files reviewed, 10 unresolved discussions (waiting on @ArniStarkware, @dafna, and @noaov1)
crates/blockifier/src/execution/syscalls/mod.rs
line 318 at r14 (raw file):
// Returns the block hash of a given block_number. // Returns the expected block hash if the given block was created at most 1024 blocks before the // latest block and at least 10 blocks before. Otherwise, returns a default value or an error
We support the entire history.
Replace 10 with a constant (next PR)
Suggestion:
// Returns the expected block hash if the given block was created at least 10 blocks before the current block. Otherwise, returns a default value or an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 23 files reviewed, 11 unresolved discussions (waiting on @ArniStarkware, @dafna, and @noaov1)
crates/blockifier/src/abi/constants.rs
line 71 at r14 (raw file):
ContractAddress::try_from(stark_felt!(1_u64)) .unwrap_or_else(|_| panic!("Failed to convert 1_u64 to StarkFelt")) });
Okay, I think that this is the definition of overkill. @giladchase wdyt?
Code quote:
pub static BLOCK_HASH_CONTRACT_ADDRESS: once_cell::sync::Lazy<ContractAddress> =
once_cell::sync::Lazy::new(|| {
ContractAddress::try_from(stark_felt!(1_u64))
.unwrap_or_else(|_| panic!("Failed to convert 1_u64 to StarkFelt"))
});
Suggestion: .unwrap_or_else(|_| panic!("Failed to convert 1_u64 to ContractAddress.")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 23 files reviewed, 12 unresolved discussions (waiting on @ArniStarkware, @dafna, @noaov1, and @Yoni-Starkware)
crates/blockifier/src/abi/constants.rs
line 71 at r14 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Okay, I think that this is the definition of overkill. @giladchase wdyt?
A bit overkill yes, also the stark_felt!
and panic
are problematic; the former is a testing feature and the later is confusing (this will never really panic so it's confusing).
We could add a const fn one()
to ContractAddress
(similar toconst fn Felt252::one
for example), does it make sense for it to be in starknet API though? (does it have special meaning for everyone implementing the API?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 23 files reviewed, 12 unresolved discussions (waiting on @ArniStarkware, @dafna, @giladchase, and @noaov1)
crates/blockifier/src/abi/constants.rs
line 71 at r14 (raw file):
Previously, giladchase wrote…
A bit overkill yes, also the
stark_felt!
andpanic
are problematic; the former is a testing feature and the later is confusing (this will never really panic so it's confusing).We could add a
const fn one()
toContractAddress
(similar toconst fn Felt252::one
for example), does it make sense for it to be in starknet API though? (does it have special meaning for everyone implementing the API?)
I don't think so. 1 is a specific contract address.
I'm okay with eiyher u64 or str for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 23 files reviewed, 12 unresolved discussions (waiting on @ArniStarkware, @dafna, @noaov1, and @Yoni-Starkware)
crates/blockifier/src/abi/constants.rs
line 71 at r14 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
I don't think so. 1 is a specific contract address.
I'm okay with eiyher u64 or str for now.
Ya, that's pretty much our go-to solution at this point as well :)
51f42e1
to
e9fd3b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 23 files reviewed, 11 unresolved discussions (waiting on @dafna, @giladchase, @noaov1, and @Yoni-Starkware)
crates/blockifier/src/abi/constants.rs
line 71 at r14 (raw file):
Previously, giladchase wrote…
Ya, that's pretty much our go-to solution at this point as well :)
Reverted.
crates/blockifier/src/execution/syscalls/mod.rs
line 294 at r14 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Why not use u64? Like in Cairo 1.
BlockNumber
is just an extension of u64
, and is more correct in this case.
There is an issue with converting from Felt to an integer type in both cases. I started thinking about it, and even created a PR in the starknet-api
repo. I now see there is a conversion that passes through usize
. Note usize
may be a u32
on some hardware.
In any case - it is not straightforward.
crates/blockifier/src/execution/syscalls/mod.rs
line 318 at r14 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
We support the entire history.
Replace 10 with a constant (next PR)
Done.
Previously, ArniStarkware (Arnon Hod) wrote…
See #604 . |
This is done. |
e9fd3b2
to
57aef41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 23 files reviewed, 11 unresolved discussions (waiting on @dafna, @giladchase, @noaov1, and @Yoni-Starkware)
crates/blockifier/src/execution/syscalls/mod.rs
line 333 at r9 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
Done.
un-reverted.
crates/blockifier/src/execution/syscalls/syscalls_test.rs
line 133 at r9 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
Done.
un-reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 12 files at r13, 10 of 13 files at r14, 5 of 7 files at r15, 2 of 2 files at r16, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dafna and @noaov1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Yoni-Starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)
This change is